Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Add mysqli connection flags to mysqli_real_connect #314

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add mysqli connection flags to mysqli_real_connect #314

wants to merge 3 commits into from

Conversation

kris-sum
Copy link

The Mysqli driver doesn't allow setting of flags passed into mysqli_real_connect.

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.

In some instances (like MySQL and PHP on Azure) MYSQLI_SSL_CLIENT needs to be set.

'params' => [
    'host' => 'xxxx.mysql.database.azure.com',
    'port' => 3306,
    'user' => 'xxxx@xxxxx',
    'password' => 'xxxxx',
    'dbname' => 'xxxx',
    'charset' => 'utf8',
    'use_ssl' => true,
    'ssl_ca' => 'xxxxx/certs/BaltimoreCyberTrustRoot.crt.pem',
    'driver_options' => [ ]
]
  • Detail the original, incorrect behavior.

This calls $this->resource->ssl_set($clientKey, $clientCert, $caCert, $caPath, $cipher); to configure the SSL options, but no flag is set on mysqli_real_connect. Connection to Azure subsequently fails with 'SSL connection is required'

  • Detail the new, expected behavior.

Driver should allow connecting as illustrated in azure documentation at https://docs.microsoft.com/en-us/azure/mysql/howto-configure-ssl .

Added 'flags' config key to driver_options to support the setting of flags into mysqli_real_connect. This lets users supply MYSQLI_CLIENT_SSL and other connection flags as indicated on http://www.php.net/manual/en/mysqli.real-connect.php

'params' => [
    'host' => 'xxxx.mysql.database.azure.com',
    'port' => 3306,
    'user' => 'xxxx@xxxxx',
    'password' => 'xxxxx',
    'dbname' => 'xxxx',
    'charset' => 'utf8',
    'use_ssl' => true,
    'ssl_ca' => 'xxxxx/certs/BaltimoreCyberTrustRoot.crt.pem',
    'driver_options' => [ 
        'flags' => MYSQLI_CLIENT_SSL 
    ]
]

@kris-sum
Copy link
Author

kris-sum commented May 8, 2018

Is anyone available to review this? @ezimuel @weierophinney ?

@michalbundyra
Copy link
Member

@kris-sum We need test case for this change.

@kris-sum
Copy link
Author

kris-sum commented May 8, 2018

@webimpress Any test case would require setting up SSL on the MySQL end - I can't see a way I can do that with the current integration test setup?

@michalbundyra
Copy link
Member

@kris-sum mock resource, pass it into the constructor and check if calls are with proper values/flags. Seems to be doable.

@ezimuel
Copy link
Contributor

ezimuel commented May 29, 2018

@kris-sum can you provide the unit test as suggested by @webimpress ? Thanks.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-db to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-db.
  • In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra michalbundyra mentioned this pull request Jan 16, 2020
Closed
4 tasks
@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#53.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kris-sum @michalbundyra @ezimuel @weierophinney